Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 133-141: The code currently logs if
document.querySelector("actionable-events") is not an instance of
ActionableEvents but then continues to dereference actionableEvents (setting
actionableEvents.eventBus, .game, .uiState and later inserting layers), causing
a crash if missing; update the guard around actionableEvents in GameRenderer
(the variable actionableEvents and type ActionableEvents) to perform an early
return or throw after the console.error so no property writes or layer insertion
run when actionableEvents is falsy/incorrect, and apply the same guarded pattern
to the other occurrence around lines where layer insertion happens (the second
actionableEvents usage) to ensure eventBus, game and uiState are only assigned
when actionableEvents is a valid ActionableEvents instance.

In `@src/core/execution/TradeShipExecution.ts`:
- Around line 192-193: Add a core simulation unit test that asserts both source
and destination port gold-bonus events are emitted when TradeShipExecution runs:
locate where TradeShipExecution calls srcPort.owner().addGold(gold,
this.srcPort.tile()) and _dstPort.owner().addGold(gold, this._dstPort.tile())
and write a test in the core simulation test suite that (1) sets up a trade
between two ports, (2) spies or subscribes to the core event/bonus emitter for
the source owner and destination owner, (3) executes the TradeShipExecution, and
(4) asserts that each owner received a gold-add event including the correct gold
amount and that the source event includes the tile context (this.srcPort.tile())
while the destination event includes this._dstPort.tile().

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/core/game/GameImpl.ts`:
- Around line 936-939: The code computes focusID by calling
this.player(focusPlayerID).smallID() without ensuring the player exists; use the
existing hasPlayer() guard to avoid the player() call when the ID is invalid.
Change the ternary to first check focusPlayerID !== undefined &&
this.hasPlayer(focusPlayerID) and only then call
this.player(focusPlayerID).smallID(); otherwise set focusID to undefined (or
handle per existing semantics), so GameImpl.player() cannot be invoked with a
missing PlayerID.
- Around line 929-930: Tests are missing for the new optional parameters on
GameImpl.displayMessage: add unit tests that call GameImpl.displayMessage (or
helper wrapper used in tests) and assert the produced GameUpdate with type
GameUpdateType.DisplayEvent contains the passed unitID as-is and the
focusPlayerID converted to smallID in the payload's DisplayEvent, and also
assert behavior when each optional parameter is undefined (i.e., absent from the
payload). Target the GameImpl.displayMessage invocation paths updated around
DisplayEvent and verify the update payload fields for unitID and focusPlayerID
(including conversion logic) in positive and undefined cases.

In `@tests/core/executions/NukeExecution.test.ts`:
- Around line 188-200: The test currently constructs a NukeExecution for
UnitType.MIRVWarhead with a null launch ref, so the detonation path isn't
exercised; update the test to create a valid missile silo (or other valid launch
unit) for 'player', obtain its reference (use game.ref(...) or the created
unit's ref) and pass that ref into new NukeExecution(UnitType.MIRVWarhead,
player, game.ref(50, 50), <siloRef>) so the MIRV launch code in NukeExecution is
executed, then run executeTicks(game, 200) and assert that game.displayMessage
was not called with NUKE_DETONATED. Ensure you reference NukeExecution,
UnitType.MIRVWarhead, and game.displayMessage when making these changes.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 184-188: The loop that cleans stale alliances (iterating
this.alliancesCheckedAt and calling removeAllianceRenewalEvents(allianceId))
doesn't call requestUpdate(), so removed renewal prompts remain visible until
another update; update the cleanup paths (the loop at the top of
ActionableEvents and the similar code around lines 76-84) to call
this.requestUpdate() after deletions (or call requestUpdate() inside
removeAllianceRenewalEvents when it actually mutates this.events) so the
component re-renders immediately when alliance renewal events are removed.

---

Outside diff comments:
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 1-326: Prettier/formatting errors are causing CI to fail; run the
project's formatter (Prettier) or apply the repo's eslint --fix/format settings
to this file so it matches the project's style. Specifically reformat the
ActionableEvents class and its methods (tick, render,
checkForAllianceExpirations, onAllianceRequestEvent, onAllianceExtensionEvent,
onBrokeAllianceEvent, addEvent/removeEvent) so spacing, indentation and line
breaks conform to the repo Prettier configuration, then re-run the linter/CI to
confirm the formatting issue is resolved.

In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 1: Formatting in EventsDisplay.ts is out of sync with project Prettier
settings; run the formatter and commit the result. Run npx prettier --write on
the file (or your repo's configured formatter) and ensure the import line and
overall file containing the EventsDisplay class/component (e.g., the import {
html, LitElement } line and the EventsDisplay class definition) are reformatted,
then stage and commit the updated formatting changes.

---

Nitpick comments:
In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 460-474: The inline <style> block defining the gold-gain-pop
keyframes and .gold-gain-pop class is acceptable in ControlPanel.ts, but if you
want reuse or avoid duplication extract the animation to a shared CSS file:
create a shared stylesheet (e.g., animations.css or shared-animations.ts)
containing the `@keyframes` gold-gain-pop and .gold-gain-pop rule, export or
import that stylesheet into ControlPanel.ts and replace the inline <style> with
an import or reference; ensure the selector name (gold-gain-pop) and keyframe
name remain unchanged so existing usage in ControlPanel (class="gold-gain-pop")
continues to work.

In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 208: The unconditional this.requestUpdate() in the EventsDisplay tick
handler causes unnecessary re-renders; remove that call and rely on the existing
update paths that already call requestUpdate() when events change (the events
update mechanism) or when _isVisible is toggled, or guard it so it only calls
requestUpdate() when the events list or _isVisible actually changed. Locate the
tick handler inside the EventsDisplay class (where requestUpdate() is called
each tick) and either delete that call or replace it with a boolean check
against the current events/visibility state (use the _isVisible property and
whatever internal events collection or last-known-events snapshot the class
uses) so renders only happen on real state changes.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 855-866: The donation messages currently use the requested
variable `gold` which can over-report the actual transferred amount; update the
two this.mg.displayMessage calls in PlayerImpl (the sender and recipient
messages) to pass `removed` instead of `gold` for the numeric value and to call
renderNumber(removed) for the formatted display; locate the calls to
this.mg.displayMessage around the donation logic (referencing recipient.id(),
this.id(), this.displayName(), renderNumber) and replace the `gold` usages with
`removed`.

---

Outside diff comments:
In `@src/core/game/UnitImpl.ts`:
- Around line 216-231: Add core unit tests that assert the new "unitID" message
argument is present for both capture and destroy flows: simulate the capture
flow in UnitImpl and the destroy flow, spy/mock the mg.displayMessage calls for
MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT (and the
destroy-related MessageType used in UnitImpl), and assert the message payload
includes unitID equal to the unit's id() (and that other payload fields remain
unchanged). Use UnitImpl to trigger the flows, intercept mg.displayMessage, and
add assertions that the emitted payload contains unitID for both the capture and
destroy scenarios.

---

Nitpick comments:
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 148-150: The code currently calls this.game.player(alliance.other)
which may throw if alliance.other refers to a non-existent player; replace that
call with this.game.playerBySmallID(alliance.other) and guard the result by
checking .isPlayer() (or handling TerraNullius) before using it—update the
variable used (e.g., other) and bail out or skip processing when
!other.isPlayer() to avoid exceptions when the referenced player was conquered
or removed.

In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 460-474: Move the inline <style> block out of
ControlPanel.render(): remove the embedded `@keyframes` gold-gain-pop and
.gold-gain-pop rules from the JSX returned by the render method and instead
declare them either in the ControlPanel component's static styles property or in
the global CSS file; update references to the animation class (gold-gain-pop) to
remain unchanged so existing uses still apply. Locate the ControlPanel class and
its render() method to remove the inline style and add the `@keyframes`
gold-gain-pop and .gold-gain-pop rules to the component's static styles (or
global stylesheet) so the animation stays available without injecting global
styles in render().

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/hud/GameRenderer.ts`:
- Line 279: In GameRenderer, ensure the element returned by the querySelector
for "actionableEvents" is validated before being appended to the layers array:
update the guard around the querySelector (the code that locates
actionableEvents) to check for null/undefined and only push actionableEvents
into layers if it is a truthy HTMLElement; if the selector fails, skip adding it
(or log/debug) so layers never contains an invalid entry. Ensure references to
actionableEvents and the layers array are updated accordingly so downstream code
always receives a valid element.

---

Outside diff comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 849-867: Add tests for donateGold to assert the displayMessage
focusPlayerID is set correctly: spy on or inspect game.displayMessage /
GameUpdateType.DisplayEvent after calling PlayerImpl.donateGold and assert the
call for MessageType.SENT_GOLD_TO_PLAYER has its last positional argument equal
to recipient.id() and the call for MessageType.RECEIVED_GOLD_FROM_PLAYER has its
last positional argument equal to donor.id() (use the PlayerImpl instance's
id()). Locate usages of donateGold, displayMessage, and
MessageType.SENT_GOLD_TO_PLAYER / MessageType.RECEIVED_GOLD_FROM_PLAYER in the
test suite, invoke donateGold with a valid amount, capture the display events,
and assert the final argument (focusPlayerID) matches the expected player id for
each message type.

In `@src/core/game/UnitImpl.ts`:
- Around line 216-232: Add unit tests that assert UnitImpl.setOwner triggers
mg.displayMessage with the unitID equal to the unit's id (this.id()).
Specifically, create tests that instantiate a UnitImpl, stub/mock its message
gateway (mg) to capture calls, call UnitImpl.setOwner (or the method containing
the shown displayMessage calls), and assert that mg.displayMessage is invoked
for MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT with
the final argument equal to the unit's id; reference UnitImpl.setOwner,
mg.displayMessage, MessageType.UNIT_CAPTURED_BY_ENEMY, and
MessageType.CAPTURED_ENEMY_UNIT when locating the code under test.
- Around line 335-342: Add a unit test that asserts
UnitImpl.displayMessageOnDeleted invokes MessageGroup.displayMessage with
MessageType.UNIT_DESTROYED and passes the deleted unit's id as the unitID
parameter: create a test that constructs a UnitImpl (or a minimal stub),
spies/mocks its mg (MessageGroup) and owner(), calls displayMessageOnDeleted (or
the code path that triggers the shown this.mg.displayMessage), and verifies the
mock received arguments including the context { unit: this._type } and that the
unitID argument equals the unit's id (this.id()). Use the existing test
harness/mock utilities to target UnitImpl.displayMessageOnDeleted,
MessageGroup.displayMessage, and MessageType.UNIT_DESTROYED.

---

Duplicate comments:
In `@src/client/hud/GameRenderer.ts`:
- Around line 124-132: The code retrieves actionableEvents and logs an error if
it's not an ActionableEvents but then unconditionally assigns
actionableEvents.eventBus, .game, and .uiState; guard those assignments by only
performing them when actionableEvents is an instance of ActionableEvents (or
early-return/throw after the error). Update the block in GameRenderer.ts around
actionableEvents so that after the instanceof check you either return/exit on
error or wrap the three assignments (eventBus, game, uiState) inside the
successful branch that confirms actionableEvents is an ActionableEvents.

In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 184-189: The cleanup loop iterating this.alliancesCheckedAt
removes stale alliance renewal events via
removeAllianceRenewalEvents(allianceId) but doesn't trigger a re-render; after
calling removeAllianceRenewalEvents(allianceId) and deleting from
this.alliancesCheckedAt, call requestUpdate() so the UI reflects the removed
prompts. Update the loop in ActionableEvents (the block referencing
this.alliancesCheckedAt and removeAllianceRenewalEvents) to invoke
requestUpdate() once a removal occurs (or once after the loop if multiple
removals) to force an update.

In `@src/core/execution/TradeShipExecution.ts`:
- Line 192: The change to TradeShipExecution using this.srcPort.tile() when
calling srcPort.owner().addGold alters the addGold event signature and requires
a core test that verifies the source-port gold bonus emits with the tile
context; add a unit test in the core test suite that constructs the minimal game
state (a srcPort with owner and tile, and a TradeShipExecution invocation or
equivalent helper), triggers the gold-award path (calling
TradeShipExecution.execute or the relevant method), and asserts that addGold was
called/emitted with both the correct gold amount and the source port's tile;
reference TradeShipExecution, srcPort.owner().addGold, and srcPort.tile() when
locating where to add the test.